Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename KeyCode to PhysicalKey #11692

Closed
wants to merge 1 commit into from

Conversation

Vrixyz
Copy link
Member

@Vrixyz Vrixyz commented Feb 4, 2024

Part of #11052

What this Pr addresses

In bevy 0.12, KeyCode was referring to logical key (user layout dependant key), as it changes in bevy 0.13, changing the naming makes this behavioral change more obvious to users.

Technically: it's just a rename of KeyCodeto PhysicalKey.

Controversial

More discussion on discord: https://discord.com/channels/691052431525675048/768253008416342076/1203993423506571285

We're copying winit's type, and winit's wording is KeyCode.

I agree keeping the same type name as winit makes it more obvious which type we're bringing in from winit. (in winit documentation, KeyCode is addressed as "Code representing the location of a physical key").

And there is a PhysicalKey deeper down the lower level.. (and KeyCode is a crossplatform abstraction over it).

Migration Guide

  • Removed KeyCode. If you were previously using KeyCode to handle logical keys (to support user's keyboard layout), you might want to reach for the event KeyboardInput and its logical_key, or the event ReceivedCharacter.
  • There is a new Res<ButtonInput<PhysicalKey>> to handle physical keys.

@@ -162,7 +162,7 @@ pub enum NativeKeyCode {
///
/// ## Usage
///
/// It is used as the generic `T` value of an [`ButtonInput`] to create a `Res<Input<KeyCode>>`.
/// It is used as the generic `T` value of an [`ButtonInput`] to create a `Res<ButtonInput<PhysicalKeyCode>>`.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tiiiny bit out of scope but probably an oversight from a previous PR.

@Vrixyz Vrixyz requested a review from maniwani February 4, 2024 09:23
@Vrixyz Vrixyz added A-Input Player input via keyboard, mouse, gamepad, and more M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 4, 2024
Copy link
Contributor

github-actions bot commented Feb 4, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@Kanabenki
Copy link
Contributor

Kanabenki commented Feb 4, 2024

Seems like the comment that's the basis of the corresponding item in #11052 mentions PhysicalKey, while the issue mentions PhysicalKeyCode. Which one should we keep?

@Vrixyz
Copy link
Member Author

Vrixyz commented Feb 4, 2024

Yeah probably right, PhysicalKey makes sense for consistency with LogicalKey

@Vrixyz Vrixyz force-pushed the 11052-rename-physical-key branch from 253f5ce to b50c1eb Compare February 4, 2024 10:55
@alice-i-cecile
Copy link
Member

If we're going to change this, I prefer PhysicalKey, with a doc alias for KeyCode. It conveys the same information, but is shorter to type and read.

@alice-i-cecile alice-i-cecile changed the title Rename KeyCode to PhysicalKeyCode Rename KeyCode to PhysicalKey Feb 4, 2024
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Feb 4, 2024
@matiqo15 matiqo15 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 4, 2024
Copy link
Contributor

@tim-blackbird tim-blackbird left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm currently eating dinner/on my phone but I'd like to avoid the logical/physical key naming scheme that winit chose. See discord

@mockersf mockersf added the X-Controversial There is active debate or serious implications around merging this PR label Feb 4, 2024
@mockersf
Copy link
Member

mockersf commented Feb 4, 2024

KeyCode seems a much better name for this as it's the key code send by the keyboard when you press a physical key, which may actually has something different printed on it

@ickk
Copy link
Member

ickk commented Feb 5, 2024

@mockersf in bevy 0.12 and prior versions KeyCode unfortunately meant "logical key", so flipping its meaning to "physical key" in 0.13 would be very surprising & confusing for users. Additionally, bevy just loosely wraps winit for input so we should follow their naming to avoid confusion, (which is "physical key" & "logical key")

@shanecelis
Copy link
Contributor

I was initially against this. But I looked at how winit arranged their PhysicalKey. They keep both KeyCode and PhysicalKey, which I think is confusing. I prefer this proposal to that.

Logical key is Key. The physical key is PhysicalKey. I can get behind that.

@CooCooCaCha
Copy link

@mockersf in bevy 0.12 and prior versions KeyCode unfortunately meant "logical key", so flipping its meaning to "physical key" in 0.13 would be very surprising & confusing for users. Additionally, bevy just loosely wraps winit for input so we should follow their naming to avoid confusion, (which is "physical key" & "logical key")

In this case I think we should consider options based on a clean slate, instead of worrying about migrations. Personally, I agree with @mockersf. KeyCode should be the code sent by the keyboard. Logical keys gets too into input mapping which I think would be better handled by a more full-featured input system like leafwing-input-manager.

Logical/physical keys seems too confusing, especially for new devs. I think KeyCode + Input Action Map is the right conceptual design.

@Vrixyz Vrixyz mentioned this pull request Feb 12, 2024
23 tasks
@alice-i-cecile
Copy link
Member

let's trust winit and go with their naming instead of adding a new layer of confusion

Closing out: I'm content to act with @mockersf as SME-Input here and make this the call.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants